Skip to content

miri: use tikv-jemalloc-sys from sysroot #149252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

miri: use tikv-jemalloc-sys from sysroot #149252

wants to merge 1 commit into from
+13 −11

Conversation

@madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Nov 24, 2025 •
edited
Loading

This allows LTO to work when compiling jemalloc (which is currently broken due to rust-lang/cc-rs#1613), which should yield a small performance boost, and makes Miri's behaviour here match Clippy
and Rustdoc.

Follow-up to #148925 / #146627 after discussion in #148925 (review).

r? RalfJung

@madsmtm madsmtm added O-linux Operating system: Linux O-macos Operating system: macOS A-allocators Area: Custom and system allocators A-miri Area: The miri tool A-linkers Area: linkers... you gotta love linkers labels Nov 24, 2025
@rustbot
Copy link
Collaborator

rustbot commented Nov 24, 2025

The Miri subtree was changed

cc @rust-lang/miri

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Nov 24, 2025
@madsmtm
Copy link
Contributor Author

madsmtm commented Nov 24, 2025

Does this need a perf run? I don't think Miri is tested in rustc-perf?

@madsmtm madsmtm mentioned this pull request Nov 24, 2025
Merged
@saethlin
Copy link
Member

saethlin commented Nov 24, 2025 •
edited
Loading

Miri has its own benchmark suite based on hyperfine that you can run with ./miri bench but that's designed for use out of rust-lang/miri so I don't know if it works in-tree.

I'd be happy merging this without running any benchmarks.

RalfJung
RalfJung reviewed Nov 24, 2025
View reviewed changes
RalfJung
RalfJung reviewed Nov 24, 2025
View reviewed changes
RalfJung
RalfJung reviewed Nov 24, 2025
View reviewed changes
src/tools/miri/src/bin/miri.rs
///
/// FIXME(madsmtm): This is loaded from the sysroot that was built with the other `rustc` crates
/// above, instead of via Cargo as you'd normally do. This is currently needed for LTO due to
/// https://github.com/rust-lang/cc-rs/issues/1613.
Copy link
Member

@RalfJung RalfJung Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also saves the time for building jemalloc again so that seems nice 🤷

Copy link
Contributor Author

@madsmtm madsmtm Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, though that's also the case for a bunch of other common dependencies like serde, libc, regex etc. If this is actually a concern, we should work on a way to share dependencies across the board instead.

Copy link
Member

@RalfJung RalfJung Nov 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tikv-jemalloc has a build script building C dependencies, making it super slow for check builds. That makes it a more worthwhile goal for this than pure Rust crates. Also the allocator is a global singleton after all so using exactly the rustc setup IMO makes a lot of sense.

@madsmtm
miri: use tikv-jemalloc-sys from sysroot
522e47f 
This allows LTO to work when compiling jemalloc, which should yield a
small performance boost, and makes miri's behaviour here match clippy
and rustdoc.
@RalfJung
Copy link
Member

RalfJung commented Nov 24, 2025

Thanks! LGTM. This does mean we won't usually use jemalloc when developing Miri in its own repo... but CI there tests with --all-features so we still cover that case at some point.

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Nov 24, 2025

📌 Commit 522e47f has been approved by RalfJung

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 24, 2025
@madsmtm
Copy link
Contributor Author

madsmtm commented Nov 24, 2025

Thanks! LGTM. This does mean we won't usually use jemalloc when developing Miri in its own repo... but CI there tests with --all-features so we still cover that case at some point.

Note that this then requires rustc to have been built with --features jemalloc as well, otherwise the sysroot may not contain tikv-jemalloc-sys (?)

@RalfJung
Copy link
Member

RalfJung commented Nov 24, 2025

Ah, we may have to cfg-out the import on targets where jemalloc is not used then... we'll probably best see that once this PR syncs into the Miri repo.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 24, 2025
@matthiaskrgr
Rollup merge of rust-lang#149252 - madsmtm:miri-jemalloc, r=RalfJung
b9fdf6a 
miri: use `tikv-jemalloc-sys` from sysroot

This allows LTO to work when compiling jemalloc (which is currently broken due to rust-lang/cc-rs#1613), which should yield a small performance boost, and makes Miri's behaviour here match Clippy
and Rustdoc.

Follow-up to rust-lang#148925 / rust-lang#146627 after discussion in rust-lang#148925 (review).

r? RalfJung
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 24, 2025
Open
bors added a commit that referenced this pull request Nov 24, 2025
@bors
Auto merge of #149276 - matthiaskrgr:rollup-wlrpdrr, r=matthiaskrgr
c871d09 
Rollup of 6 pull requests

Successful merges:

 - #148234 (rustdoc: make mergeable crate info more usable)
 - #149201 (Add suggest alternatives for Out-of-range \x escapes)
 - #149208 ([rustdoc] Make more functions return `fmt::Result` and reduce number of `.unwrap()` calls)
 - #149252 (miri: use `tikv-jemalloc-sys` from sysroot)
 - #149255 (Use `let...else` consistently in user-facing diagnostics)
 - #149275 (Fix missing double-quote in `std::env::consts::OS` values)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@RalfJung RalfJung

Labels

A-allocators Area: Custom and system allocators A-linkers Area: linkers... you gotta love linkers A-miri Area: The miri tool O-linux Operating system: Linux O-macos Operating system: macOS S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@madsmtm @rustbot @saethlin @RalfJung @bors